Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update FCS, allow fsdocs to roll forward to net5.0 #621

Merged
merged 19 commits into from
Jan 11, 2021
Merged

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Dec 1, 2020

  1. Update FCS to 38.0.0
  2. Add MSBuild dependencies requried for 38.0.0
  3. Move consistently to netstandard2.1
  4. allow fsdocs to roll forward to net5.0

@dsyme
Copy link
Contributor Author

dsyme commented Dec 1, 2020

This requires dotnet/fsharp#10575 to pass on .NET 5.0.

Until then fsdocs requires an install of .NET Core App 3.1

@cartermp
Copy link
Contributor

Note that this will require FCS 38.0.1, which we can actually depend on today if we use the public feed instead

@dsyme dsyme closed this Dec 22, 2020
@dsyme dsyme reopened this Dec 22, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

@cartermp The way we're doing FCS is a problem here.

  - FSharp.Compiler.Service 38.0.1-beta.20607.1 requested package FSharp.Core: 5.0.1-beta.20607.1 (beta)*

We need to

  1. always build FCS against a public FSharp.Core
  2. have FCS use an FSharp.Core from a few versions back to enable improved reach for the FCS ecosystem

cc @baronfel

dotnet/fsharp#10778

@cartermp
Copy link
Contributor

Hmm, I disagree

@cartermp
Copy link
Contributor

agains 5.0.x maybe, but not a few versions back, that would disallow editors from supporting F# 5 features for example

@cartermp
Copy link
Contributor

Or would it? I dunno. Not worth it though. There's little value in backtargeting an FCS that supports F# X but it reuquires an FSharp.Core that is against X-Y. Reach is not really a thing here. People can just use older FCS versions if they must runa gainst older F# compilers

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

agains 5.0.x maybe, but not a few versions back, that would disallow editors from supporting F# 5 features for example

Why? The API would be built against 4.7.2 but could run against any later FSharp.Core.

If truly reflective FCS-based tooling wants to have their final tool use 5.0.0 that's fine.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

Well, at the moment we can certainly get away with a 5.0.0 API dependency but it should always be something public and on nuget.org. It should not be latest in-repo FSharp.Core beta.

Forcing latest dependencies becomes a bigger problem to the extent that

  1. FCS is binary compatible and has an ecosystem of components on top of it and/or

  2. We have binary compatible slices of FCS for https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1033-analyzers.md and/or

  3. The particular existing binary compat ecosystem of FSharp.Analyzers

As an example, the current dependency chains are

FSharp.Core -->
Microsoft.DotNet.DependencyManager -->
FSharp.Compiler.Service --> 
FSharp.Analyzers SDK -->
individual F# analyzers

FSharp.Core -->
Microsoft.DotNet.DependencyManager -->
FSharp.Compiler.Service --> 
FSharp.Formatting --> 
Ionide  

The emerging set up means that we are forcing that whole chain to to update to latest and greatest, which both forces references to dotnet/fsharp nuget feed betas, and breaks all Ionide F# Analyzers on every update even if FSharp.Compiler.Service were actually binary compatible. That's not the destination we want.

The principles of FCS have been as follows:

  • FCS is a library component which shouldn't force dependencies on all the latest and greatest but have a baseline dependency set that gets updated slowly

  • Its update publication/release should be quick and not be delayed until latest dotnet/fsharp everything is published.

  • It is netstandard2.0 to allow reach (like all libraries) and not force false latest dependencies

  • It uses stable, public, lower FSharp.Core to not force false latest dependencies (like all libraries)

  • It uses SemVer

  • It is not yet binary compatible but aspires to be in the future

TBH I think the problem is that dotnet/fsharp has a long cultural habit of forcing latest FSharp.Core on all components it builds - it's like it seems to happen almost automatically in Microsoft repos. The build-from-source in that repo may also part of this (because access to a package server can't be assumed). But it also feels like it's a general engineering/mindset problem - it's so noticeable that as soon as we start building FCS from that repo all the existing principles get upended (and suddenly I'm on the verge of forcing whacky nuget sources and latest beta dependencies on the entire downstream chain for Ionide...)

Note many of the same arguments apply to TPSDK - which should have a low FSharp.Core dependency to allow people to continue to make TPs that work with existing F# tooling

So in short dotnet/fsharp needs to build FCS and TPSDK without having them be FSharp.Core latest.

We also have to get Microsoft.DotNet.DependencyManager published and rely on a stable published version of that, I think that's appreciated too. That should also have a public, stable, low FSharp.Core dependency to allow dependency managers to be built for a range of F# tooling.

@cartermp
Copy link
Contributor

I don't really agree with all of that. We'll have to agree to disagree I think.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

I don't really agree with all of that. We'll have to agree to disagree I think.

Hmmm :-) Let's talk this over at some point, and I'd like to know specifically what you don't agree with.

The issue of dependency management is fundamental to all of FCS, TPSDK, DependencyManager, F# Analyzers. There's simply no point having extension hooks if every update breaks (or requires re-compilation of) all extensions.

I also can't resolve this particular issue we're discussing (#621) until the dependency chain for FCS is on nuget.org. I'm not going to ask Ionide and other downstream users to add nuget sources nor beta FSharp.Core references. A beta FCS reference would be ok at a pinch (since FCS is in many ways a beta thing anyway)

@cartermp
Copy link
Contributor

The issue of dependency management is fundamental to all of FCS, TPSDK, DependencyManager, F# Analyzers. There's simply no point having extension hooks if every update breaks all extensions.

That's actually my point. These are not normal packages. It's been explained to me that things like FSharp.Core is "just" a normal library, when it...literally is required to compile any F# code, and depending on the code, it may not even compile unless you have the right version. Anything involving the analysis of F# source code is not a normal component and cannot be treated as such. Being able to resolve nuget dependencies requires an up-to-date msbuild and nuget component to be able to resolve things correctly, for example. And the "language" for parsing these things out isn't understood by older F# language versions. So why on earth would we want to somehow find a way for older consumers to use this stuff? That's hellish and falls apart the instant anything updates.

A beta FCS reference would be ok at a pinch (since FCS is in many ways a beta thing anyway)

This I think is reasonable, though still problematic since we have this issue where some features are partially implemented in FSharp.Core. Analysis of byrefs wouldn't be possible prior to FSharp.Core 4.5.2, for example. I can see a similar issue arising at some point. But I think it's mostly workable.

Although it's worth pointing out that in figuring out publishing FCS from dotnet/fsharp, we literally did discuss this and folks were fine with taking on a dependency on nightly releases of FCS via a nuget feed so they could get the latest bits. We've provided that.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

Hmmm maybe I see the disconnect here.

The static FSharp.Core dependency of FCS is irrelevant to the actual operation of FCS.

In this sense FCS is indeed a normal package with resepect to its static FSharp.Core dependency. It is just a minbound on the surface area of FSharp.Core that FCS needs to run - just as a static dependency of "netstandard2.0" is a minbound on the surface area of .NET that FCS needs to run. It is irrelevant to the language features supported.

For example, if FCS statically has a minimum static FSharp.Core 4.7.2 dependency that literally doesn't mean anything about the F# code it can and can't process. Zilch. If I write an F# static analysis tool using FCS 39.0.0 (and any compatible FSharp.Core) then it can do any combination of things

  • Analyse F# 4.7 netstandard2.1 code using a compile-time reference to FSharp.Core 5.0.0
  • Analyse F# 5.0 netcoreapp3.1 code using a compile-time reference to FSharp.Core 5.0.0
  • Analyse F# 5.0 net5 code using a compile-time reference to FSharp.Core 4.7.2 (with some lang features getting auto-disabled by FCS langfeature logic due to runtime support not being present in referenced FSharp.Core)

The runtime FSharp.Core in an FCS-based tool (e.g. a test suite or fsdocs) is relevant to the operation of FCS, but only for reflective operations (specifically script execution and the default FSharp.Core reference for processing scripts).

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

Although it's worth pointing out that in figuring out publishing FCS from dotnet/fsharp, we literally did discuss this and folks were fine with taking on a dependency on nightly releases of FCS via a nuget feed so they could get the latest bits. We've provided that.

Yes, in principle I'm "just" ok with this part - FCS is beta and if we really need to occasionally beta-ize the whole dependent ecosystem then I guess that's ok. As I mentioned at the start I know there are a lot of benefits to Microsoft doing the publishing from dotnet/fsharp. I guess I'd be much happier if those betas were on nuget.org. Requiring downstream users (Ionide) to add unusual nuget sources to use latest FSharp.Formatting is not a happy experience. That's just not the kind of component FSharp.Formatting wants to be.

However I've always stressed that we need FCS (and Microsoft.DotNet.DependencyManager and TPSDK) to statically depend on well-understood, public minbound-surface-area FSharp.Core. I actually thought we had that already looking at https://github.com/dotnet/fsharp/blob/main/src/fsharp/Directory.Build.props#L10 but that doesn't apply to the CI pakages we're building.

I care about the second issue much more than the first.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 22, 2020

To summarize the versions of FSharp.Core relevant to the operation of FSharp.Compiler.Service:

  1. The FSharp.Compiler.Service.dll static reference to FSharp.Core - The FCS DLL and nuget have a static minbound dependency on FSharp.Core.

    This is just a normal .NET dependency like any other, it expresses the minimum surface area of FSharp.Core that the implementation of FSharp.Compiler.Service (and any components that depend on it) needs. It could be a reference to a reference assembly if we supported that. In theory this could be very low and all is cool - if we could implement FCS in terms of FSharp.Core 2.0.0.0 then that could be the minbound (indeed in theory we could implement FCS pretty almost without any use of FSharp.Core functionality at all, though obviously we don't)

    In practice this should be 1-2 versions behind latest FSharp.Core. At the moment 5.0.0 would be fine.

  2. The runtime reference to FSharp.Core in a tool, application or test suite that includes FSharp.Compiler.Service - This is the actual version of FSharp.Core used when, say, fsc.exe or devenv.exe or fsi.exe or fsdocs.exe runs.

    This must be at least as high as (1) and is usually the very latest FSharp.Core available (in or out of repo tree). This is important to the operation of the FCS-based tool because it is used for execution of scripts, and the default compilation reference for scripts. If scripts are going to use a particular language feature then this must be sufficient to support the language feature

  3. The FSharp.Core reference in a compilation or analysis being processed by FSharp.Compiler.Service.

    This can be anything - 2.0.0.0, 4.0.0.0 or 5.0.0 or whatever. For script compilation and execution is is the same as (2). It must be sufficient to support language features used in the compilation.

I've added these notes to dotnet/fsharp#10772

@cartermp
Copy link
Contributor

We managed to get all complaince-related stuff sorted for nuget, so now there are regular updates to fCS flowing again. https://www.nuget.org/packages/FSharp.Compiler.Service/ has everything you need to unblock fsharp.formatting + ionide.

I'm be very curious to see if the inclusion of the nuget dependency manager also now means that F# scripts processed via fsharp.formatting could include tooltip info from those nuget references...

@dsyme
Copy link
Contributor Author

dsyme commented Jan 11, 2021

We managed to get all complaince-related stuff sorted for nuget, so now there are regular updates to fCS flowing again. https://www.nuget.org/packages/FSharp.Compiler.Service/ has everything you need to unblock fsharp.formatting + ionide.

@cartermp This is great news, thank you so much, I'll update now

I'm be very curious to see if the inclusion of the nuget dependency manager also now means that F# scripts processed via fsharp.formatting could include tooltip info from those nuget references...

Looking at the dependencies of https://www.nuget.org/packages/FSharp.Compiler.Service/38.0.2 I don't see Microsoft.DotNet.DependencyManager in that list, I think it's because the files are still included directly in the FCS project? Would we expect that to work? I haven't tried it yet

@baronfel
Copy link
Collaborator

It all works as expected, we're using 38.0.2 successfully in FSAC with #r nuget support.

@dsyme
Copy link
Contributor Author

dsyme commented Jan 11, 2021

It all works as expected, we're using 38.0.2 successfully in FSAC with #r nuget support.

Cool thanks. I'd imagine plugin dependency resolvers e.g. #r "paket: ..." don't work (since the extensions are binary components built against Microsoft.DotNet.DependencyManager)

@baronfel
Copy link
Collaborator

That's right. We'd consider bundling a paket one for convenience I think, but the overall mechanism of handler registration needs something more fleshed out to ensure version consistency between dotnet fsi and tooling like FCS, in my opinion.

@cartermp
Copy link
Contributor

@dsyme I meant using #r "nuget". I don't know why you'd use #r "paket" for examples TBH

@dsyme dsyme merged commit c92b005 into fsprojects:master Jan 11, 2021
@nhirschey nhirschey mentioned this pull request Dec 3, 2023
@smoothdeveloper
Copy link

@cartermp, we could use #r "paket:" for paket documentation or when nuget doesn't work (NuGet/Home#13013), it is a life saver 🙂, people should be able to use it without people saying what you say about it IMO.

I think we can stop with the "why do people use paket", just accepting that there are people who do, it will never hurt F#; thinking it does is, I think a bit problematic (like if I said if people use C#, it hurts F# or the reverse).

@baronfel, I take your word on bundling #r "paket:", we are 2 PR aways in context of ionide (and FSAC tooling):

@cartermp
Copy link
Contributor

cartermp commented Dec 4, 2023

I don't think I understand. The issue you linked would still affect #r "paket:..." yes? Since it's related to fetching stuff from nuget's backend, which is what paket uses.

@smoothdeveloper
Copy link

to clarify, I just came for understanding #890 (I think we should allow consumers that are netstandard2.0) and picked up two random comments in the long thread for this herculean effort (that was to bring the repo to .net core) along the way, no issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants